Send Percy screenshots via npm script (not Jest) #3009

colinrotherham · 2022-11-14T18:29:53Z

This PR moves our Percy screenshots out of Jest following a recent conversation

Discussion regarding #2864: “I was originally thinking of trying to split the Percy screenshotting out from Jest as the screenshot ‘tests’ aren’t really tests at all.”

This has added another big speed up to our checks

colinrotherham · 2022-11-15T11:38:29Z

.github/workflows/tests.yml

@@ -140,7 +140,13 @@ jobs:

          - description: JavaScript component tests
            name: test-component
-            run: npx percy exec -- jest --color --selectProjects "JavaScript component tests"
+            run: npx jest --color --maxWorkers=2 --selectProjects "JavaScript component tests"


Without the CPU load from the @percy/puppeteer Chromium tabs we can now run JavaScript component tests with 2x workers

We use the same flag for JavaScript behaviour tests above

colinrotherham · 2022-11-15T12:54:02Z

.github/workflows/tests.yml

+        uses: ./.github/workflows/actions/install-node
+
+      - name: Start review application
+        run: npm run serve &


Starts the review app in the background using &

colinrotherham · 2022-11-15T12:55:07Z

.github/workflows/tests.yml

+
+    env:
+      PERCY_TOKEN: ${{ secrets.PERCY_TOKEN }}
+      PORT: 8888


Review app uses port 3000 but our Puppeteer helpers want 8888

36degrees · 2022-11-18T10:49:04Z

.github/workflows/tests.yml

-          PERCY_TOKEN: ${{ secrets.PERCY_TOKEN }}
+
+  regression:
+    name: Regression checks


Can we make it clearer that the checks happen 'in Percy' rather than as part of this action? Maybe naming this something like 'Send screenshots to Percy'?

Ah so this is the job name "Regression checks" which might include others in future?

The step name on line 189 says "Send screenshots to Percy"

Happy to make them match though

36degrees · 2022-11-18T10:49:28Z

.github/workflows/tests.yml

@@ -137,10 +137,12 @@ jobs:
          - description: JavaScript behaviour tests
            name: test-behaviour
            run: npx jest --color --maxWorkers=2 --selectProjects "JavaScript behaviour tests"
+            cache: .cache/jest


Is this related to the other changes?

This was me noticing that we restore the .cache/jest directory for all four Verify jobs

But didn't necessarily need it for:

npm run build:package npm run build:dist

As we saw in the Windows runners often "restoring the cache" was slower than not having it.

The code was simpler before so I'll put it back

36degrees · 2022-11-18T10:58:37Z

lib/puppeteer-helpers.js

@@ -2,7 +2,7 @@ const { componentNameToJavaScriptClassName } = require('./helper-functions.js')
 const { renderHtml } = require('./jest-helpers.js')

 const configPaths = require('../config/paths.js')
-const PORT = configPaths.ports.test
+const { PORT = configPaths.ports.test } = process.env


I find this quite hard to read compared to e.g.

Suggested change

const { PORT = configPaths.ports.test } = process.env

cost PORT = process.env.PORT || configPaths.port

Ah sorry you've said this before, I'll change it 👍

We've got a couple of others:

https://github.com/alphagov/govuk-frontend/blob/main/config/jest/server/start.mjs#L6
https://github.com/alphagov/govuk-frontend/blob/main/lib/puppeteer-helpers.js#L5

It looks clearer with multiple envs perhaps?

const { ENV_1 = 'Fallback value 1', ENV_2 = 'Fallback value 2', ENV_3 = 'Fallback value 3', ENV_4 = 'Fallback value 4', ENV_5 = 'Fallback value 5' } = process.env

36degrees · 2022-11-18T10:59:00Z

tasks/screenshot-components.mjs

+ * @param {string} screenshotName - Screenshot name
+ * @returns {Promise<void>}
+ */
+export async function screenshot (page, screenshotName) {


Is there a benefit to wrapping percySnapshot?

For JSDoc perhaps at the time, let's remove it

36degrees · 2022-11-18T11:00:03Z

tasks/screenshot-components.mjs

+  // Screenshot each component
+  const input = [...componentNames]
+
+  // Screenshot 4x components concurrently


How did you arrive at 4x? Might be useful to capture in the commit message in case we want to revisit in the future.

Tried 5x initially but 4x was faster

Good point though, worth trying 2x and 3x as well (you never know!)

If you click into the Send screenshots to Percy job to see the timings:

1x component in sequence govuk-frontend/actions/runs/5854565851 – ⏱️ 36 seconds

2x components concurrently govuk-frontend/actions/runs/3496480973 – ⏱️ 31 seconds

3x components concurrently govuk-frontend/actions/runs/3496481529 – ⏱️ 28 seconds

4x components concurrently govuk-frontend/actions/runs/3496493071 – ⏱️ 22 seconds

5x components concurrently govuk-frontend/actions/runs/5854480472 – ⏱️ 22 seconds

The total action time varies due to GitHub cache retrieval API times being wobbly

I'll keep it at 4x but add a comment

Runs faster than “one at a time” by using multiple Chromium tabs. Once we go above 4x though we see little extra benefit See: #3009 (comment)

36degrees

I think we can tidy up a dependency we no longer need, but otherwise this looks good to me 👍🏻

36degrees · 2022-11-18T11:51:09Z

docs/releasing/testing-and-linting.md

@@ -101,7 +101,7 @@ We generate 2 screenshots for each default example of every component. One examp

 The screenshots are public, so you can check them without logging in. A BrowserStack account is needed to approve or reject any changes (if you don't have access, ask your tech lead for help). If you're the reviewer of the pull request code, it's your responsibility to approve or request changes for any visual changes Percy highlights.

-When you run the tests locally (for example, using `npm test`), Percy commands are ignored and Percy does not generate any screenshots. You will see the following message in your command line output: `[percy] Percy is not running, disabling snapshots`.
+When you run the tests locally (for example, using `npm run test:screenshots`), Percy commands are ignored and Percy does not generate any screenshots. You will see the following message in your command line output: `[percy] Percy is not running, disabling snapshots`.


Thanks for keeping the docs in sync 🙌🏻

@36degrees No problem. I didn't quite know if we should keep the test:* prefix

Based on the "…‘tests’ aren’t really tests” feedback

But we can always revisit another day. Thanks again 😊

36degrees · 2022-11-18T11:56:14Z

src/govuk/components/all.test.js

@@ -1,51 +0,0 @@
-const { join } = require('path')
-const { fetch } = require('undici')
-const { WebSocket } = require('ws')


Can we now remove ws from our dependencies? (Was introduced in 496cc38)

Very nice, yep let's do that 🎉

Runs faster than “one at a time” by using multiple Chromium tabs. Once we go above 4x though we see little extra benefit See: #3009 (comment)

colinrotherham added the Frontend squad label Nov 14, 2022

colinrotherham requested a review from a team as a code owner November 14, 2022 18:29

colinrotherham added this to Needs review 🔍 in Design System Sprint Board via automation Nov 14, 2022

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3009 November 14, 2022 18:30 Inactive

colinrotherham force-pushed the move-percy-screenshots branch from 21626e5 to 4a7a526 Compare November 15, 2022 09:16

colinrotherham changed the base branch from review-app-esm to main November 15, 2022 09:16

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3009 November 15, 2022 09:16 Inactive

colinrotherham force-pushed the move-percy-screenshots branch from 4a7a526 to 7194636 Compare November 15, 2022 10:46

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3009 November 15, 2022 10:46 Inactive

colinrotherham force-pushed the move-percy-screenshots branch from 7194636 to be60cac Compare November 15, 2022 10:56

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3009 November 15, 2022 10:56 Inactive

colinrotherham changed the title ~~Send Percy screenshots via npm script~~ Send Percy screenshots via npm script (not Jest) Nov 15, 2022

colinrotherham force-pushed the move-percy-screenshots branch from be60cac to 026b54c Compare November 15, 2022 11:18

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3009 November 15, 2022 11:18 Inactive

colinrotherham commented Nov 15, 2022

View reviewed changes

colinrotherham force-pushed the move-percy-screenshots branch from 026b54c to 9765f9e Compare November 15, 2022 12:48

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3009 November 15, 2022 12:48 Inactive

colinrotherham force-pushed the move-percy-screenshots branch from 9765f9e to c99ce8d Compare November 15, 2022 12:50

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3009 November 15, 2022 12:50 Inactive

colinrotherham commented Nov 15, 2022

View reviewed changes

colinrotherham force-pushed the move-percy-screenshots branch from c99ce8d to c4dbaab Compare November 15, 2022 12:59

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3009 November 15, 2022 12:59 Inactive

colinrotherham force-pushed the move-percy-screenshots branch from c4dbaab to 5f4204c Compare November 15, 2022 13:06

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3009 November 15, 2022 13:07 Inactive

colinrotherham force-pushed the move-percy-screenshots branch from 5f4204c to 568ca1a Compare November 15, 2022 13:13

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3009 November 15, 2022 13:13 Inactive

colinrotherham mentioned this pull request Nov 15, 2022

Add (optional) GitHub Action checkbox for runner OS #2945

Merged

36degrees reviewed Nov 18, 2022

View reviewed changes

colinrotherham force-pushed the move-percy-screenshots branch from 568ca1a to 28ad3fb Compare November 18, 2022 11:20

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3009 November 18, 2022 11:21 Inactive

colinrotherham added a commit that referenced this pull request Nov 18, 2022

Send Percy screenshots in batches

398dee5

Runs faster than “one at a time” by using multiple Chromium tabs. Once we go above 4x though we see little extra benefit See: #3009 (comment)

colinrotherham force-pushed the move-percy-screenshots branch from 28ad3fb to 398dee5 Compare November 18, 2022 11:38

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3009 November 18, 2022 11:39 Inactive

36degrees approved these changes Nov 18, 2022

View reviewed changes

colinrotherham added 2 commits November 18, 2022 12:03

Send Percy screenshots via npm script

fd46daa

Send Percy screenshots in batches

93cde6f

Runs faster than “one at a time” by using multiple Chromium tabs. Once we go above 4x though we see little extra benefit See: #3009 (comment)

colinrotherham force-pushed the move-percy-screenshots branch from 398dee5 to 93cde6f Compare November 18, 2022 12:03

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3009 November 18, 2022 12:03 Inactive

colinrotherham merged commit cdfc281 into main Nov 18, 2022

Design System Sprint Board automation moved this from Needs review 🔍 to Done 🏁 Nov 18, 2022

colinrotherham deleted the move-percy-screenshots branch November 18, 2022 12:14

This was referenced Nov 21, 2022

Skip “Percy screenshots” when secrets are unavailable on forks #3028

Merged

Bug: Percy uploads should complete before “finalizing” #3036

Merged

colinrotherham added assurance github-actions labels Feb 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send Percy screenshots via npm script (not Jest) #3009

Send Percy screenshots via npm script (not Jest) #3009

colinrotherham commented Nov 14, 2022 •

edited

colinrotherham Nov 15, 2022

colinrotherham Nov 15, 2022

colinrotherham Nov 15, 2022 •

edited

36degrees Nov 18, 2022

colinrotherham Nov 18, 2022

36degrees Nov 18, 2022

colinrotherham Nov 18, 2022

36degrees Nov 18, 2022

colinrotherham Nov 18, 2022

36degrees Nov 18, 2022

colinrotherham Nov 18, 2022

36degrees Nov 18, 2022

colinrotherham Nov 18, 2022

colinrotherham Nov 18, 2022

36degrees left a comment

36degrees Nov 18, 2022

colinrotherham Nov 18, 2022

36degrees Nov 18, 2022

colinrotherham Nov 18, 2022

	const { PORT = configPaths.ports.test } = process.env
	cost PORT = process.env.PORT \|\| configPaths.port

Send Percy screenshots via npm script (not Jest) #3009

Send Percy screenshots via npm script (not Jest) #3009

Conversation

colinrotherham commented Nov 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinrotherham Nov 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

36degrees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinrotherham commented Nov 14, 2022 •

edited

colinrotherham Nov 15, 2022 •

edited